Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Store extended attributes in files in stead of in extended attributes #3649

Merged
merged 39 commits into from
Feb 21, 2023
Merged

Store extended attributes in files in stead of in extended attributes #3649

merged 39 commits into from
Feb 21, 2023

Conversation

aduffeck
Copy link
Contributor

@aduffeck aduffeck commented Feb 9, 2023 •
edited by butonic
Loading

We added a new metadata backend for the decomposed storage driver that uses an additional .ini file to store file metadata. This allows scaling beyond some filesystem specific xattr limitations.

@update-docs
Copy link

update-docs bot commented Feb 9, 2023

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

@aduffeck aduffeck changed the base branch from master to edge February 9, 2023 14:22
@aduffeck aduffeck force-pushed the materialized-xattrs branch 5 times, most recently from b03f553 to f4a1bbe Compare February 13, 2023 10:19
@butonic butonic marked this pull request as ready for review February 16, 2023 16:51
@butonic
more linter fixes
d0c3392 
Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
@butonic
always initialize xattr backend to make tests pass
866388a 
Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
butonic
butonic approved these changes Feb 16, 2023
View reviewed changes
Copy link
Contributor

@butonic butonic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yuck, the missing abstraction of a node causes a lot of noise and making the linter happy causes even more noise. The prefix usage is sprinkled over all decomposedfs files ... we really need to move that to the node as in 87a0d23 to truly decouple the implementation. The current xattr backend interface only ties the code further together by introducing methods like IsMetaFile(path string) bool, UsesExternalMetadataFile() bool and especially MetadataPath(path string) string.

I'm ok with merging it as is, as it introduces a new experimental backend for metadata which frees us from xattr limitations in some corner cases. But before touching this again we should refactor this mess ASAP.

@butonic butonic changed the title [WIP] Store extended attributes in files in stead of in extended attributes Store extended attributes in files in stead of in extended attributes Feb 16, 2023
@butonic butonic mentioned this pull request Feb 16, 2023
Closed
C0rby
C0rby approved these changes Feb 20, 2023
View reviewed changes
Copy link
Contributor

@C0rby C0rby left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Just one small nitpick

pkg/storage/utils/decomposedfs/tree/tree.go Outdated Show resolved Hide resolved
kobergj
kobergj approved these changes Feb 20, 2023
View reviewed changes
Copy link
Contributor

@kobergj kobergj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍 But can we run a [full-ci] pipeline with this changes on ocis before merging? If you did already please ignore the comment.

@butonic
use global var for regex
3169cbd 
Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
@butonic butonic mentioned this pull request Feb 21, 2023
Merged
@butonic
Copy link
Contributor

butonic commented Feb 21, 2023

LGTM 👍 But can we run a [full-ci] pipeline with this changes on ocis before merging? If you did already please ignore the comment.

running in https://drone.owncloud.com/owncloud/ocis/19330

@butonic
Copy link
Contributor

butonic commented Feb 21, 2023

ocis full-ci green, too https://drone.owncloud.com/owncloud/ocis/19330

@butonic butonic merged commit 573ae44 into cs3org:edge Feb 21, 2023
@labkode
Copy link
Member

labkode commented Feb 22, 2023

Hi, I just want to give my 2 cents on this.

Keeping a metadata file in sync with the file is referring to is very hard task. Atomicity is paramount to avoid race conditions.

  • How are you preventing two concurrent updates to the metadata of the file?
  • The use of flock is process-dependant. The storage layer dealing with files is usually one single process . How do you flock.
  • The use of flock it's has always a pain in the ass on top of NFS (what enterprises usually want). fnctl should be used in these cases.

@micbar micbar mentioned this pull request Feb 27, 2023
Closed
@kobergj kobergj mentioned this pull request May 22, 2023
Closed
@kobergj kobergj mentioned this pull request Mar 1, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kobergj kobergj kobergj approved these changes

@C0rby C0rby C0rby approved these changes

@butonic butonic butonic approved these changes

@labkode labkode Awaiting requested review from labkode labkode is a code owner

@ishank011 ishank011 Awaiting requested review from ishank011

@glpatcern glpatcern Awaiting requested review from glpatcern glpatcern is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@aduffeck @butonic @labkode @C0rby @kobergj